Create config and cache files with 0600 permissions#2573
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR centralizes hardened-permission writes into a single config::locator::write_hardened_file helper and migrates several config/cache writers to use it, ensuring newly created files land with 0600 permissions on Unix (instead of relying on umask defaults). This directly addresses the reported issue where the background upgrade-check would create upgrade_check.json with relaxed permissions and trigger a subsequent “updated permissions” warning on the next run.
Changes:
- Added
locator::write_hardened_fileand updated multiple writers to use it instead of plainfs::write/ manualOpenOptionshandling. - Added a Unix-only regression test asserting
upgrade_check.jsonis created with0600permissions. - After streaming downloads in
snapshot create, appliesset_hardened_permissionspost-rename(best-effort) to harden the final cached file.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| cmd/soroban-cli/src/config/upgrade_check.rs | Uses write_hardened_file for saving; adds Unix permission regression test for 0600. |
| cmd/soroban-cli/src/config/mod.rs | Switches Config::save_to to write_hardened_file after ensure_directory. |
| cmd/soroban-cli/src/config/locator.rs | Introduces write_hardened_file helper and migrates existing writers to it. |
| cmd/soroban-cli/src/config/data.rs | Writes actionlog/spec cache artifacts via write_hardened_file. |
| cmd/soroban-cli/src/commands/tx/edit.rs | Writes the editor scratch file via write_hardened_file (while keeping tempdir 0700 on Unix). |
| cmd/soroban-cli/src/commands/snapshot/create.rs | Applies set_hardened_permissions to cached downloads after rename (best-effort). |
leighmcculloch
approved these changes
May 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Centralize hardened-permission file writes through a new
locator::write_hardened_filehelper, and migrate every config/cache writer to use it. On Unix the helper opens withOpenOptions + mode(0o600)and callsset_hardened_permissionsafterwards; on non-Unix it falls back tostd::fs::write.Migrated writers:
KeyType::write,UpgradeCheck::save,Config::save_to,data::write,data::write_spec,save_contract_id,remove_contract_id, andtx edit's scratch file. Forsnapshot create's two streaming bucket downloads (which can't take a byte slice) added aset_hardened_permissionscall after eachfs::rename. User-owned destinations (--out,--out-file, WASM build artifacts) are intentionally left alone.Closes #2562.
Why
UpgradeCheck::savewroteupgrade_check.jsonwith plainfs::write, so on first creation the file landed at the umask default (typically0644). Because the upgrade check runs in the background on every CLI invocation, the next run'sensure_directorysweep walked the tree, fixed the perms, and printed⚠️ Updated config files permissions to 0600.to users who had not run any write-style command. The same shape existed in several other writers (Config::save_to,data::write/write_spec, etc.) and was being papered over byfix_config_permissionsafter the fact. Going through one helper closes the bug class instead of just the reported instance.Known limitations
snapshot createhave a brief window between rename and chmod where the file exists at the umask-default mode. Acceptable since the contents are public ledger data, not secrets.set_hardened_permissionserrors after the streaming downloads are intentionally swallowed, matching the soft-failure behavior already used byfix_config_permissions.